Skip to content

(HOLD) OPRUN-3941: Add webhook tests with enhancements and fixes #430

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

camilamacedo86
Copy link
Contributor

@camilamacedo86 camilamacedo86 commented Aug 13, 2025

From: #434
- Skips the test that is failing. We will fix it in a follow-up. We need #436 in the payload.
- Add dumping of container logs and kubectl describe pods output for better diagnostics.
- Include targeted certificate details dump (tls.crt parse) when failures occur.
- Add additional check to verify webhook responsiveness after certificate rotation.

This change is a refactor with fixes of the code from openshift/origin#30059.

/payload-aggregate periodic-ci-openshift-release-master-ci-4.20-e2e-gcp-ovn-techpreview-serial 10

Testhing with payload and GCP to ensure that the error: https://prow.ci.openshift.org/view/gs/test-platform-results/logs/periodic-ci-shiftstack-ci-release-4.20-techpreview-e2e-openstack-ovn-serial-techpreview/1955816149002227712

Will not be faced

Copy link
Contributor

openshift-ci bot commented Aug 13, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: camilamacedo86
Once this PR has been reviewed and has the lgtm label, please assign oceanc80 for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@camilamacedo86 camilamacedo86 changed the title [OTE] simplify webhook tests, improve names and logs WIP: [OTE] simplify webhook tests, improve names and logs Aug 13, 2025
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 13, 2025
@camilamacedo86 camilamacedo86 force-pushed the follow-up-enhance-ote branch 3 times, most recently from 158497e to 44e43f7 Compare August 13, 2025 09:48
@camilamacedo86 camilamacedo86 changed the title WIP: [OTE] simplify webhook tests, improve names and logs OPRUN-3941: [OTE] simplify webhook tests, improve names and logs Aug 13, 2025
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 13, 2025
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Aug 13, 2025
@openshift-ci-robot
Copy link

openshift-ci-robot commented Aug 13, 2025

@camilamacedo86: This pull request references OPRUN-3941 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.20.0" version, but no target version was set.

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

Group: "webhook.operators.coreos.io",
Version: "v2",
Resource: "webhooktests",
}

func newWebhookTestV1(name, namespace string, valid bool) *unstructured.Unstructured {
func newWebhookTest(name, namespace string, valid bool) *unstructured.Unstructured {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, this is not anything major at all, it was just bothering me that this function returns a webhook, so that we can use that in our tests, so newTestWebhook seems more of an appropriate name, than newWebhookTest. newWebhookTest means this is a test itself, which it is not.

Copy link
Contributor Author

@camilamacedo86 camilamacedo86 Aug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the common standard is New<StructName|Kind|Class>
In this case, the kind inside of the sample/project that we use to do the test is "kind": "WebhookTest",
so, therefore, make sense newWebhookTest

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are asking for a NEW struct of WebhookTest

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fmt.Fprintf(GinkgoWriter, "\n[pod-logs] namespace=%s\n", namespace)

By("Getting all pods in the namespace")
namesOut, err := RunK8sCommand(ctx, "get", "pods", "-n", namespace, "-o", "name")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a cli tool (kubectl/oc) to get pod logs? Can't we just use controller runtime client to get all the pods?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QE tests will need to use the CLI so here we add the helper already for them
And in this case is more cleaner the code with oc/kubectl then with the API
I mean, the code is KISS. Otherwise, we need add more extra deps

@tmshort
Copy link
Contributor

tmshort commented Aug 13, 2025

/test openshift-e2e-aws

1 similar comment
@camilamacedo86
Copy link
Contributor Author

/test openshift-e2e-aws

Copy link
Contributor

openshift-ci bot commented Aug 15, 2025

@camilamacedo86: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command

  • periodic-ci-openshift-release-master-ci-4.20-e2e-gcp-ovn-techpreview-serial

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/84478cd0-79f7-11f0-868c-f17f1c5c9461-0

@tmshort
Copy link
Contributor

tmshort commented Aug 15, 2025

Test #436 is a cert fix for our upstream-e2e, and has nothing to do with OTE, so I'm not sure why you're referencing it?

@camilamacedo86 camilamacedo86 changed the title OPRUN-3941: [OTE] Add webhook tests with enhancements and fixes (HOLD)OPRUN-3941: [OTE] Add webhook tests with enhancements and fixes Aug 15, 2025
@camilamacedo86
Copy link
Contributor Author

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 15, 2025
@camilamacedo86
Copy link
Contributor Author

Test #436 is a cert fix for our upstream-e2e, and has nothing to do with OTE, so I'm not sure why you're referencing it?

Because the test that we faced issues in Sippy is should be tolerant to openshift-service-ca certificate rotation ( techpreview ), we deleted the secret, and we expect that it will be re-created and be valid.

Therefore, I wanted to test it with your PR.
Anyway, see: https://redhat-internal.slack.com/archives/C06KP34REFJ/p1755276543066359?thread_ts=1755273190.823239&cid=C06KP34REFJ

@camilamacedo86
Copy link
Contributor Author

/payload-aggregate periodic-ci-openshift-release-master-ci-4.20-e2e-gcp-ovn-techpreview-serial 10

Copy link
Contributor

openshift-ci bot commented Aug 16, 2025

@camilamacedo86: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command

  • periodic-ci-openshift-release-master-ci-4.20-e2e-gcp-ovn-techpreview-serial

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/9db3f980-7a77-11f0-9317-77fddb6f96d1-0

@camilamacedo86
Copy link
Contributor Author

/test openshift-e2e-aws

@camilamacedo86
Copy link
Contributor Author

/payload-aggregate periodic-ci-openshift-release-master-ci-4.20-e2e-gcp-ovn-techpreview-serial 10

(we need check if after the cert-fix it is either fixed)

Copy link
Contributor

openshift-ci bot commented Aug 18, 2025

@camilamacedo86: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command

  • periodic-ci-openshift-release-master-ci-4.20-e2e-gcp-ovn-techpreview-serial

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/389f9120-7bf9-11f0-8801-38b19603745e-0

… certificate rotation

This change is a refactor of code from openshift/origin#30059.

Assisted-by: Gemini
@camilamacedo86 camilamacedo86 changed the title (HOLD)OPRUN-3941: [OTE] Add webhook tests with enhancements and fixes (HOLD)OPRUN-3941: Add webhook tests with enhancements and fixes Aug 19, 2025
@camilamacedo86 camilamacedo86 changed the title (HOLD)OPRUN-3941: Add webhook tests with enhancements and fixes (HOLD) OCPBUGS-60564: Add webhook tests with enhancements and fixes Aug 19, 2025
@openshift-ci-robot openshift-ci-robot added jira/severity-important Referenced Jira bug's severity is important for the branch this PR is targeting. jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Aug 19, 2025
@openshift-ci-robot
Copy link

@camilamacedo86: This pull request references Jira Issue OCPBUGS-60564, which is invalid:

  • expected the bug to target the "4.20.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

From: #434

  • Skips the test that is failing. We will fix it in a follow-up. We need NO-ISSUE: Handle service-ca cert availability/rotation #436 in the payload.
  • Add dumping of container logs and kubectl describe pods output for better diagnostics.
  • Include targeted certificate details dump (tls.crt parse) when failures occur.
  • Add additional check to verify webhook responsiveness after certificate rotation.

This change is a refactor with fixes of the code from openshift/origin#30059.

/payload-aggregate periodic-ci-openshift-release-master-ci-4.20-e2e-gcp-ovn-techpreview-serial 10

Testhing with payload and GCP to ensure that the error: https://prow.ci.openshift.org/view/gs/test-platform-results/logs/periodic-ci-shiftstack-ci-release-4.20-techpreview-e2e-openstack-ovn-serial-techpreview/1955816149002227712

Will not be faced

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@camilamacedo86 camilamacedo86 changed the title (HOLD) OCPBUGS-60564: Add webhook tests with enhancements and fixes (HOLD) OPRUN-3941: Add webhook tests with enhancements and fixes Aug 19, 2025
@openshift-ci-robot openshift-ci-robot removed jira/severity-important Referenced Jira bug's severity is important for the branch this PR is targeting. jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Aug 19, 2025
@openshift-ci-robot
Copy link

openshift-ci-robot commented Aug 19, 2025

@camilamacedo86: This pull request references OPRUN-3941 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.20.0" version, but no target version was set.

In response to this:

From: #434

  • Skips the test that is failing. We will fix it in a follow-up. We need NO-ISSUE: Handle service-ca cert availability/rotation #436 in the payload.
  • Add dumping of container logs and kubectl describe pods output for better diagnostics.
  • Include targeted certificate details dump (tls.crt parse) when failures occur.
  • Add additional check to verify webhook responsiveness after certificate rotation.

This change is a refactor with fixes of the code from openshift/origin#30059.

/payload-aggregate periodic-ci-openshift-release-master-ci-4.20-e2e-gcp-ovn-techpreview-serial 10

Testhing with payload and GCP to ensure that the error: https://prow.ci.openshift.org/view/gs/test-platform-results/logs/periodic-ci-shiftstack-ci-release-4.20-techpreview-e2e-openstack-ovn-serial-techpreview/1955816149002227712

Will not be faced

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@camilamacedo86
Copy link
Contributor Author

Closing I will open a new PR to faciliate the review and analise with the test that we do not add due the issues faced, more info: https://redhat-internal.slack.com/archives/C06KP34REFJ/p1755607157127779?thread_ts=1755602572.169319&cid=C06KP34REFJ

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants